perf(scanner): cache immutable scan-root path context - #899
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
Changes스캔 경로 및 인증 규칙 변경
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant cmd_scan
participant build_scan_path_context
participant _scan_file
participant ScanPathContext
cmd_scan->>build_scan_path_context: 스캔 루트 분류 및 컨텍스트 생성
build_scan_path_context-->>cmd_scan: ScanPathContext 반환
cmd_scan->>_scan_file: 각 파일과 path_context 전달
_scan_file->>ScanPathContext: relative_candidate 호출
ScanPathContext-->>_scan_file: 표시 및 필터링 경로 반환
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review @opencode-agent Review-only request for exact current head All nine exact-head repository workflows are complete and successful, including the dedicated 42/42 statement path-context gate; there are zero unresolved threads. Please independently verify the immutable one-context batch path, standalone fallback, public path semantics, authentication-comment precision, and the operation-count-only performance claim. Submit only a formal exact-head verdict. Do not mutate the branch, self-approve, merge, tag, publish, release, or weaken gates. |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Exact refs and merge base =='
git rev-parse HEAD
git rev-parse 3e7cec7b539fef7b14ae54133511acfe7d82df1b
git rev-parse 0d07baae44a40edfcaec5e42c7fb9351510ca9f0
git merge-base 3e7cec7b539fef7b14ae54133511acfe7d82df1b 0d07baae44a40edfcaec5e42c7fb9351510ca9f0
echo '== Changed files and summary =='
git diff --check 0d07baae44a40edfcaec5e42c7fb9351510ca9f0...3e7cec7b539fef7b14ae54133511acfe7d82df1b
git diff --stat 0d07baae44a40edfcaec5e42c7fb9351510ca9f0...3e7cec7b539fef7b14ae54133511acfe7d82df1b
git diff --name-status 0d07baae44a40edfcaec5e42c7fb9351510ca9f0...3e7cec7b539fef7b14ae54133511acfe7d82df1b
echo '== Relevant symbol locations =='
rg -n -C 4 -g '*.py' \
'\b(ScanPathContext|build_scan_path_context|cmd_scan|_scan_file)\b' .
echo '== Relevant authentication-pattern locations =='
rg -n -i -C 4 -g '*.py' \
'(auth(entication)?|credential|defer|comment)' .
echo '== Candidate test and workflow files =='
fd -t f . | rg '(^|/)(test|tests)/|\.github/workflows/|pyproject\.toml$|coverage'
echo '== Changed patch =='
git diff --find-renames --find-copies --no-ext-diff \
0d07baae44a40edfcaec5e42c7fb9351510ca9f0...3e7cec7b539fef7b14ae54133511acfe7d82df1bLength of output: 154956 🏁 Script executed: #!/bin/bash
set -euo pipefail
log=/tmp/coderabbit-shell-logs/shell-output-LZd3gJ
echo '== Exact-ref section from prior inspection =='
sed -n '1,80p' "$log"
echo '== CLI function map =='
ast-grep outline scanner/cli/appguardrail.py --items all --type function --match 'cmd_scan|_scan_file|_path_allowed_by_rule|_display_path'
echo '== Core context source =='
cat -n appguardrail_core/scan_paths.py
echo '== Changed CLI call path =='
sed -n '1380,1455p' scanner/cli/appguardrail.py
sed -n '2890,3015p' scanner/cli/appguardrail.py
echo '== YAML rule loading and regex evaluation =='
rg -n -C 12 -g '*.py' \
'(safe_load|yaml\.|pattern-regex|patterns|applicable_rules|finditer)' \
scanner appguardrail_core
echo '== Current packaged authorization rules =='
cat -n scanner/rules/authz.yml
echo '== Base packaged authorization rules =='
git show 0d07baae44a40edfcaec5e42c7fb9351510ca9f0:scanner/rules/authz.yml | cat -n
echo '== Focused test maps and contents =='
ast-grep outline tests/test_scan_path_context_core.py --items all --type function
ast-grep outline tests/test_scan_path_context_integration.py --items all --type function
ast-grep outline tests/test_auth_deferral_comment_rule.py --items all --type function
sed -n '1,180p' tests/test_scan_path_context_core.py
sed -n '1,180p' tests/test_scan_path_context_integration.py
sed -n '1,140p' tests/test_auth_deferral_comment_rule.pyLength of output: 58950 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Packaged-rule consumers and full-YAML engines =='
rg -n -C 5 \
'(scanner/rules|authz\.yml|--config|semgrep.*config|rules_dir|rules_path|glob.*\.yml|rglob.*\.yml)' \
. -g '!scanner/rules/authz.yml'
echo '== References to modified rule identities =='
rg -n -C 4 \
'\b(api-route-missing-auth-check|missing-api-auth|db-query-without-user-filter|req-body-user-id-trust|trusting-client-user-id)\b' \
.
echo '== Parser and packaged-rule loader =='
sed -n '930,1045p' scanner/cli/appguardrail.py
sed -n '1045,1105p' scanner/cli/appguardrail.py
rg -n -C 8 -g '*.py' \
'\b(_parse_yaml_regex_rules|_load.*rule|SCAN_RULES\s*=|scanner/rules)\b' \
scanner/cli/appguardrail.py
echo '== Path normalization and scan-root setup =='
sed -n '1145,1185p' scanner/cli/appguardrail.py
sed -n '1345,1395p' scanner/cli/appguardrail.py
echo '== Diff limited to non-comment packaged-rule changes =='
git diff --no-ext-diff --unified=12 \
0d07baae44a40edfcaec5e42c7fb9351510ca9f0...3e7cec7b539fef7b14ae54133511acfe7d82df1b \
-- scanner/rules/authz.ymlLength of output: 33177
I reviewed One blocking finding remains.
The immutable batch context, standalone fallback, path semantics, and operation-count-only claim are consistent with the reviewed source. I did not rerun repository code in this read-only environment.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scanner/rules/authz.yml`:
- Around line 53-54: The patterns in scanner/rules/authz.yml lines 53-54
currently produce duplicate findings for line-comment markers embedded inside a
block comment; make the matchers mutually exclusive or combine them so each
comment yields exactly one todo-skip-auth finding. Add a test case in
tests/test_auth_deferral_comment_rule.py lines 54-69 containing an indented “//
TODO ... auth” inside a block comment and assert that exactly one finding is
reported.
- Around line 13-24: Update the two authz.yml pattern-not-inside rules to
restrict $AUTH and $SESSION to the approved authentication API and result
symbols, rather than matching arbitrary await calls such as fetch. Require
authentication to occur before any data access, so later authentication calls
cannot bypass the rule, and add regression coverage for both arbitrary await and
data-access-before-auth cases.
In `@tests/test_scan_path_context_core.py`:
- Around line 55-56: Update the pytest.raises assertion around
build_scan_path_context to use a raw, escaped regex pattern so the dot in
“pathlib.Path” matches a literal period and validates the exact TypeError
message.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d96063d1-e44c-422f-9eda-3aa00c8b8a60
📒 Files selected for processing (12)
.github/workflows/scan-path-context-coverage.ymlCHANGELOG.d/893-scan-path-context.mdappguardrail_core/__init__.pyappguardrail_core/scan_paths.pydocs/scanner-path-context.mdscanner/cli/appguardrail.pyscanner/rules/authz.ymltests/test_appguardrail.pytests/test_auth_deferral_comment_rule.pytests/test_scan_path_context_core.pytests/test_scan_path_context_integration.pytests/test_scan_path_context_release_contract.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/test_authz_rule_contract.py (1)
24-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value원문 텍스트 단정은 서식 변경에 취약합니다.
Line 40과 Line 41의 단정은 16칸 들여쓰기를 문자열에 포함합니다. YAML을 재정렬하거나 들여쓰기를 바꾸면 동작 변경이 없어도 테스트가 실패합니다. 또한 Line 33의
assert "await fetch" not in rule은 약한 음성 검사입니다. 주석이나 메시지 텍스트에await fetch가 들어가면 실패합니다.가능하면 YAML을 파싱해 구조 단위로 단정하십시오. 예를 들어
patterns항목을 읽어metavariable-regex와 승인된 인증 호출 집합을 비교하십시오. 서식 독립성이 확보됩니다.참고:
scanner/rules/authz.yml에서 인증 결과 바인딩을 메타변수로 바꾸는 리팩터를 적용하면 Line 27-32와 Line 40-41도 함께 갱신해야 합니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_authz_rule_contract.py` around lines 24 - 43, Update the tests in test_authz_rule_contract.py to parse the authz rule YAML and assert its structural patterns instead of matching indentation-sensitive raw strings. Validate the metavariable-regex and approved authentication-call pattern set directly, and replace the broad “await fetch” absence check with a structure-aware assertion. Keep the late-auth rule assertions focused on parsed pattern relationships and update bindings consistently if scanner/rules/authz.yml changes its authentication result metavariable.scanner/rules/authz.yml (1)
118-347: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift조합 폭발로 규칙 유지보수 비용이 큽니다.
두 규칙은 핸들러 시그니처 2종과 승인된 인증 API 7종을 수동으로 곱해 총 42개의 패턴 블록을 만듭니다. 승인된 인증 API를 하나 추가하면 6개 블록을 추가해야 합니다. 누락 시 오탐이 발생합니다.
pattern-not-inside안에서pattern-either를 사용하거나, 인증 호출 대상에metavariable-regex를 적용해 축을 하나로 줄이십시오.♻️ 축소 예시
- pattern-not-inside: | export async function $METHOD(...) { ... const $SESSION = await $AUTH(...) ... $DB.$QUERY(...) ... } - metavariable-regex: metavariable: $AUTH regex: '^(?:getSession|getServerSession|auth|requireAuth|authenticate|verifySession|supabase\.auth\.getUser)$'이 축소가 기존 테스트 계약을 유지하는지 확인하십시오.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scanner/rules/authz.yml` around lines 118 - 347, Refactor the auth-after-data-access rule’s duplicated handler-signature and authentication-call pattern blocks into metavariables: use a shared handler pattern with the existing $REQ/$RES or typed-request forms, bind the authentication function as $AUTH, and constrain it with metavariable-regex to the seven approved APIs. Apply the same abstraction in pattern-not-inside, preserving the existing $DB regex and test behavior while ensuring newly added approved APIs require only one regex update.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_authz_rule_contract.py`:
- Line 8: Update the RULE_PATH definition in the authz rule contract test to
resolve scanner/rules/authz.yml relative to the test file’s location, not the
process working directory. Use the test module’s __file__ path and preserve the
existing read_text behavior.
---
Nitpick comments:
In `@scanner/rules/authz.yml`:
- Around line 118-347: Refactor the auth-after-data-access rule’s duplicated
handler-signature and authentication-call pattern blocks into metavariables: use
a shared handler pattern with the existing $REQ/$RES or typed-request forms,
bind the authentication function as $AUTH, and constrain it with
metavariable-regex to the seven approved APIs. Apply the same abstraction in
pattern-not-inside, preserving the existing $DB regex and test behavior while
ensuring newly added approved APIs require only one regex update.
In `@tests/test_authz_rule_contract.py`:
- Around line 24-43: Update the tests in test_authz_rule_contract.py to parse
the authz rule YAML and assert its structural patterns instead of matching
indentation-sensitive raw strings. Validate the metavariable-regex and approved
authentication-call pattern set directly, and replace the broad “await fetch”
absence check with a structure-aware assertion. Keep the late-auth rule
assertions focused on parsed pattern relationships and update bindings
consistently if scanner/rules/authz.yml changes its authentication result
metavariable.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 07cf38bd-956c-49ce-8d35-269197a8ac0a
📒 Files selected for processing (4)
CHANGELOG.d/893-scan-path-context.mdscanner/rules/authz.ymltests/test_authz_rule_contract.pytests/test_scan_path_context_core.py
🚧 Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.d/893-scan-path-context.md
- tests/test_scan_path_context_core.py
Scope
Closes #893 by moving scan-root classification and normalized relative-path context into one immutable reusable core record, then reusing that exact record across every file in a scan without changing established path semantics. The same branch also resolves exact-head authentication-rule review findings discovered while validating the scanner rule pack.
Implementation
ScanPathContextandbuild_scan_path_contextas standalone core contracts;cmd_scan, builds one frozen context, and passes the same object to every_scan_filecall;_scan_fileconsumers;str,strsubclasses,Path, dotfiles, separator boundaries, prefix collisions, symlinks, directory roots, and single-file behavior;Exact-head evidence
Current source head:
dd69eeb99ebca01f32625e74b5b88213f26ec14eCurrent PR-base snapshot:
c395459ed20cf47207cf5f8842f3c78a3a4c1298The path-context implementation was locally verified before the review-remediation commits with:
git diff --check: clean.The deterministic performance claim is deliberately limited to operation counts: a representative 10,000-file scan reduces scan-root classification from 10,000 operations to 1. No wall-clock percentage is claimed.
All three exact CodeRabbit findings have been addressed and their threads are resolved. Hosted workflows for the current head are queued/pending; earlier-head green results are not treated as current-head evidence.
Delivery state
The PR remains open for current-head CI/security validation and independent review. It must not merge until exact-head required checks, live governance, and any required independent approval are satisfied.
Summary by CodeRabbit
새 기능
버그 수정
문서